Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(ws): use controller-runtime for backend #43

Merged
merged 2 commits into from
Sep 14, 2024
Merged

feat(ws): use controller-runtime for backend #43

merged 2 commits into from
Sep 14, 2024

Conversation

thesuperzapper
Copy link
Member

@thesuperzapper thesuperzapper commented Sep 11, 2024 •
edited
Loading

This PR sets up the Workspace Backend API to use controller-runtime rather than ClientSets to access the Kubernetes API.

This has some significant benefits:

  1. We can use the local envtest for mocking API responses locally (like we already do in the controller tests)
  2. Rather than constantly querying the API, it will use a "watch cache" based approach, so that we only list the resources once (e.g. Workspaces), and then watch for changes.
  3. For extremely fast lookup of complicated queries like "list all workspaces owned by workspace kind xxxx", we can use indexes on the cache, like we already do for the controller.

The way this is implemented is a little hacky, because we are directly adding a new "Runnable" to the controller-runtime manager object, and this runnable encapsulates our internal net/http server.

Other than that, this PR makes the following changes:

  1. Loads the Workspace/WorkspaceKind types using a replace github.com/kubeflow/notebooks/workspaces/controller => ../controller redirect in the go.mod
  2. Adds controller-runtime to go.mod
  3. Moves all non-api go code under internal/
  4. Makes the /api/v1/healthcheck/ path return a list of Workspace names (lol, we can revert this before merging).
  5. Renames the local variable for App from app to a

@thesuperzapper
feat(ws): use controller-runtime with backend
5d0595a 
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
@thesuperzapper thesuperzapper requested review from ederign and removed request for kimwnasptd September 11, 2024 06:17
@thesuperzapper thesuperzapper added the project/notebooks-v2 project - kubeflow notebooks v2 label Sep 11, 2024
@thesuperzapper
Copy link
Member Author

Also, the tests won't pass until we either remove the dependency on the client being up (in the health-check test), or do exactly what we have done in the controller, and set up the controller-manager:

notebooks/workspaces/controller/internal/controller/suite_test.go

Lines 98 to 105 in bc4e445

By("setting up the controller manager")
k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme.Scheme,
Metrics: metricsserver.Options{
BindAddress: "0", // disable metrics serving
},
})
Expect(err).NotTo(HaveOccurred())

We also will need to add the github.com/onsi/gomega testing framework as a dependency of the backend.

@ederign ederign mentioned this pull request Sep 14, 2024
Closed
@ederign
Copy link
Member

ederign commented Sep 14, 2024

Rebased and updated here: #44

@thesuperzapper
revert healthcheck to normal results
9aa3297 
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
@ederign
Copy link
Member

ederign commented Sep 14, 2024

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Sep 14, 2024
@thesuperzapper
Copy link
Member Author

/approve

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ederign, thesuperzapper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit b0af8ae into kubeflow:notebooks-v2 Sep 14, 2024
3 checks passed
@thesuperzapper thesuperzapper deleted the backend-with-controller-runtime branch September 14, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ederign ederign ederign approved these changes

Assignees

@ederign ederign

Labels
approved lgtm project/notebooks-v2 project - kubeflow notebooks v2 size/L
Projects
Kubeflow Notebooks
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thesuperzapper @ederign